Closed
Bug 594964
Opened 14 years ago
Closed 14 years ago
nsDOMDataTransfer creates nsDOMFiles with no document
Categories
(Core :: DOM: Copy & Paste and Drag & Drop, defect)
Core
DOM: Copy & Paste and Drag & Drop
Tracking
()
VERIFIED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: sgreenlay, Assigned: khuey)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
2.84 KB,
text/html
|
Details | |
7.53 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
See test case. Under FF3.6 dragging multiple files onto 'Drop Here' will pass all files to handleFiles(), under trunk it only passes the first file
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
Keywords: regression
Comment 1•14 years ago
|
||
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd13b6ce36bd&tochange=67aef7ffb282 I'll bet money this is a regression from bug 583863. And that this should block b6.
Blocks: 583863
Assignee | ||
Comment 2•14 years ago
|
||
Uh oh! I'll take a look at this today. Shame we didn't catch this with tests :-(
Assignee | ||
Comment 3•14 years ago
|
||
This has nothing to do with multiple files: Error: uncaught exception: [Exception... "Access to restricted URI denied" code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)" location: "https://bug594964.bugzilla.mozilla.org/attachment.cgi?id=473754 Line: 121"]
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 4•14 years ago
|
||
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/dev/mo zilla-central3/obj-i686-pc-mingw32/netwerk/base/src/../../../../netwerk/base/src /nsIOService.cpp, line 610 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/dev/mo zilla-central3/obj-i686-pc-mingw32/content/base/src/../../../../content/base/src /nsDOMFileReader.cpp, line 516 JavaScript error: , line 0: uncaught exception: [Exception... "Access to restric ted URI denied" code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)" loc ation: "https://bug594964.bugzilla.mozilla.org/attachment.cgi?id=473754 Line: 12 1"] nsFileDataProtocolHandler::NewChannel is failing because it can't find the information it needs to create a new channel.
Assignee | ||
Comment 5•14 years ago
|
||
Confirmed that if I back out 583863 this is fixed.
Assignee | ||
Comment 6•14 years ago
|
||
All 583863 did was expose this by using the URL to implement the FileReader. The real issue is that nsDOMDataTransfer creates files with a null document. We don't register the file with the mozfiledata protocol handler when the document is null because nothing will be around to unregister us (we really should just throw NS_ERROR_FAILURE instead). This may or may not be a regression, I haven't tested anything but my 583863-less build yet.
Summary: REGRESSION: Multiple file drop gives only first file → nsDOMDataTransfer creates nsDOMFiles with no document
Assignee | ||
Updated•14 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 7•14 years ago
|
||
This works, I'm just not sure whether or not it's safe. The idea is that if the drag originates from outside Gecko we make the owning document the current document. This leads us both to actually register the URI and give it a principal that makes sense. I also cleaned up some weakref stuff that's really not necessary since a DOMFile should never outlive its parent document.
Attachment #474416 -
Flags: superreview?(bzbarsky)
Attachment #474416 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #474416 -
Flags: review? → review?(jonas)
Assignee | ||
Updated•14 years ago
|
Keywords: regression
Updated•14 years ago
|
blocking2.0: ? → betaN+
Assignee | ||
Comment 8•14 years ago
|
||
This is probably responsible for the drag and drop test at http://studio.html5rocks.com/ not working on trunk.
I don't understand. Why doesn't the target document work? Also, what guarentees that the nsDOMFile outlives its document?
Assignee | ||
Comment 12•14 years ago
|
||
targetDoc doesn't work because mDragTarget is always null if the file came from outside of Gecko. This is essentially Bug 572139, just exposed differently because FileReader is now implemented using File.url. It's possible I'm wrong about the nsDOMFile outliving its document, but as I understand it the only things that can keep it alive are references from script and the owning reference that nsFileDataProtocolHandler holds when a url is asked for. Those owning references are freed when the document is destroyed. I can remove that part of the patch if you like.
Blocks: 572139
Assignee | ||
Comment 13•14 years ago
|
||
Actually, MXR leads me to believe that mDragTarget is *always* null. I'll investigate how internal drops function.
Assignee | ||
Comment 14•14 years ago
|
||
Comment on attachment 474416 [details] [diff] [review] Patch Clearing reviews until I can investigate further.
Attachment #474416 -
Flags: superreview?(bzbarsky)
Attachment #474416 -
Flags: review?(jonas)
(In reply to comment #12) > targetDoc doesn't work because mDragTarget is always null if the file came from > outside of Gecko. This is essentially Bug 572139, just exposed differently > because FileReader is now implemented using File.url. Oh, I guess I was thinking of the event-target, not the drop target. I.e. the node that the drop event is fired at. Can we get to the event from data-transfer? > ...Those owning references are freed when the document is destroyed. Where? The only thing I know of that is freed when the document is destroyed is the entries in the url->file hash. But that doesn't affect the nsDOMFile object. > I can remove that part of the patch if you like. I would prefer that yes.
Assignee | ||
Comment 16•14 years ago
|
||
Yeah, this is broken for an internal drag too. Try dragging from the download manager to the test case. mDragTarget is always null. It appears that when nsDOMDataTransfer was implemented nobody knew what mDragTarget and AddElement was supposed to do ...
Assignee | ||
Comment 17•14 years ago
|
||
I'll write a test for this later today.
Attachment #474416 -
Attachment is obsolete: true
Attachment #475540 -
Flags: review?(jonas)
Assignee | ||
Comment 18•14 years ago
|
||
I meant to s/sourceDoc/doc/, consider that done.
Assignee | ||
Comment 19•14 years ago
|
||
Testing this is proving to be more difficult than I expected, probably won't get to it till this weekend.
I'm still wondering about the question in comment 15: Can we get to the event from data-transfer?
Assignee | ||
Comment 21•14 years ago
|
||
Nope, everything dealing with events is handled in nsEventStateManager and not passed to the nsDOMDataTransfer.
Assignee | ||
Comment 22•14 years ago
|
||
The only reason we ever want the document at all is because the document is responsible for unregistering URIs that we gave to scripts through file.url. What if instead of doing this we simply create the file objects with a nsIPrincipal reference instead of a nsIDocument reference and then in nsDOMFile::GetUrl() we can call GetDocumentFromCaller() to figure out who should unregister the url.
Actually, file.url exists no more. But we still use the document to figure out a principal for the url. But we can change that by making turning internalUrl into a function and make it take an nsIDocument or nsIPrincipal parameter.
Assignee | ||
Updated•14 years ago
|
Attachment #475540 -
Attachment is obsolete: true
Attachment #475540 -
Flags: review?(jonas)
Assignee | ||
Comment 24•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #476446 -
Flags: review?(jonas)
Comment on attachment 476446 [details] [diff] [review] : Make nsDOMFile's principal interact properly with drag and drop. Please add an NS_ENSURE_STATE or similar check to make sure we bail if aPrincipal is null in GetInternalUrl. r=me with that.
Attachment #476446 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 26•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a660900a793f
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•